Add Promises to RTMClient start and disconnect
#611
Conversation
Codecov Report
@@ Coverage Diff @@
## master #611 +/- ##
==========================================
+ Coverage 88.83% 92.01% +3.18%
==========================================
Files 7 7
Lines 421 426 +5
Branches 73 78 +5
==========================================
+ Hits 374 392 +18
+ Misses 31 19 -12
+ Partials 16 15 -1
Continue to review full report at Codecov.
|
| // delegate behavior to state machine | ||
| this.stateMachine.handle('explicit disconnect'); | ||
| // delegate behavior to state machine | ||
| this.stateMachine.handle('explicit disconnect'); |
There was a problem hiding this comment.
any reason to kick this event off inside the returned Promise's constructor, as opposed to the line before the return statement?
There was a problem hiding this comment.
If the web socket immediately errors on closing (that is, on the same tick of the event loop as handle is called) then the event handler won't be registered by the time the state machine transitions, and the promise will never reject.
Alternatively, the web socket could also immediately close (e.g. when mocked for testing) and the same problem would happen, though the promise wouldn't resolve.
I'm just being cautious that the reactive bit of code is set up before I set off the chain reaction. Don't want any race conditions happening 馃槄
There was a problem hiding this comment.
馃憣sounds reasonable to me!
src/RTMClient.ts
Outdated
| this.logger.debug(`transitioning to state: ${state}`); | ||
| // Emits events: `disconnected`, `connecting`, `connected`, 'disconnecting', 'reconnecting' | ||
| this.emit(state); | ||
| // event payload is anything related to the event, i.e. an error on disconnect | ||
| this.emit(state, context.eventPayload); |
There was a problem hiding this comment.
it seems like this could have effects greater than passing through the error on disconnect. i'd like to be a little more explicit about what this argument will contain for each kind of transition.
There was a problem hiding this comment.
Just looking through the code, the only other case I could see where something else passes through would be the event payloads for web socket events, specifically the non-existent open event's payload and the web socket closing code and reason from the close event.
I agree, though, that this could end up leaking something else through. Is there a better course of action? Like guarding that only eventPayload instanceof Error passes?
There was a problem hiding this comment.
it seems like the intention is to only emit the context.eventPayload when transitioning to `"disconected". if that's true, i think it would be clearer to do something like:
if (state === 'disconnected') {
// when arriving at this state due to failure, the eventPayload will contain an error.
// otherwise, like after a force disconnect, this value will be undefined
const errorOrUndefined = context.eventPayload;
this.emit(state, errorOrUndefined);
} else {
this.emit(state);
}its more explicit, and gives us a chance to document the intention.
|
@clavin looks like we're really close! i'd like to make a release soon, since #619 adds a type definition change that would be helpful. if you're willing the address the last outstanding comment by Friday, i'll hold off for that. otherwise i'll release at latest then, and we can continue to work on getting this PR landed. |
|
thanks! |
Summary
Fixes #198 and #610:
connectingsubmachine to transition to afailedstate, which emits afailureevent on the parent machine, allowing for thedisconnectedstate to be reached from a failure to reconnect (tl;dr: it works 馃憤)RTMClient#starttoPromise<WebAPICallResult>andRTMClient#disconnecttoPromise<void>Requirements